Skip to content

Conversation

@k3DW
Copy link
Contributor

@k3DW k3DW commented Sep 11, 2025

Follow-up of #81

@pdimov
Copy link
Member

pdimov commented Oct 6, 2025

Let's leave BOOST_INSTALL_INCLUDE_SUBDIR alone and (always, regardless of layout) install into CMAKE_INSTALL_DATADIR/boost-1.90.0.

I'll take a look at the rest later.

@k3DW
Copy link
Contributor Author

k3DW commented Oct 6, 2025

It makes more sense to have matching installation for include/ and extra/. Using BOOST_INSTALL_INCLUDE_SUBDIR avoids that issue. For example, either we get include/boost-1_90 and share/boost-1_90, or we get simply include and share. I think they should always match.

@pdimov
Copy link
Member

pdimov commented Oct 6, 2025

No, I don't think it does. The include subdir is maintained for consistency with b2 install, new things (e.g. the CMake config files) use the -1.90.0 convention.

I don't think that dumping all the libs/libname/extra directories into /usr/share is what we want (or what the user would have wanted.) We can afford to do this for libs/libname/include only because their contents always have a boost subdirectory, usually boost/libname subdirectory, and collisions are checked by b2 headers. We don't have any established conventions for the contents of extra, or any enforcement or checks for those nonexistent conventions.

So to avoid dumping random things into /usr/share, we should install in a subdirectory, and to avoid libraries stepping over one another's toes, we probably should install into boost_libname-1.90.0, like the CMake configs.

@k3DW
Copy link
Contributor Author

k3DW commented Oct 6, 2025

Understood, I can make that change, and remove the configurability from it.

@k3DW k3DW force-pushed the natvis branch 2 times, most recently from da55c3f to 9d467f4 Compare October 9, 2025 04:11
@pdimov
Copy link
Member

pdimov commented Oct 10, 2025

You have legitimate CI failures if you haven't noticed:

-- Adding Boost library assert
CMake Error at /Users/runner/work/cmake/boost-root/tools/cmake/include/BoostInstall.cmake:273 (message):
  boost_install_target: both or neither of EXTRA_DIRECTORY and
  EXTRA_INSTALL_DIRECTORY must be given.
Call Stack (most recent call first):
  /Users/runner/work/cmake/boost-root/tools/cmake/include/BoostInstall.cmake:606 (boost_install_target)
  /Users/runner/work/cmake/boost-root/tools/cmake/include/BoostRoot.cmake:202 (boost_install)
  /Users/runner/work/cmake/boost-root/tools/cmake/include/BoostRoot.cmake:421 (__boost_auto_install)
  /Users/runner/work/cmake/boost-root/CMakeLists.txt:20 (include)

That's probably because when e.g. BOOST_SKIP_INSTALL_RULES is set, EXTRA_INSTALL_DIRECTORY is empty, and your check in boost_install_target precedes the check for BOOST_SKIP_INSTALL_RULES (because boost_install_target does more than just install, and those other things aren't skipped when BOOST_SKIP_INSTALL_RULES is set.)

@pdimov
Copy link
Member

pdimov commented Oct 11, 2025

Thank you Braden for your work on this. I think this is ready to merge and will do so a bit later.

@k3DW
Copy link
Contributor Author

k3DW commented Oct 11, 2025

Great thank you

@pdimov pdimov merged commit fd61b23 into boostorg:develop Oct 18, 2025
457 checks passed
@pdimov
Copy link
Member

pdimov commented Oct 18, 2025

I'm playing with this, and I notice that the installed Assert doesn't have the .natvis file - because it's added as a PRIVATE source. Fair enough. When I make it PUBLIC, it appears in the imported target's sources, and the path to it seems correct.

But this gets me thinking. I also add the headers to Boost::assert, also as PRIVATE. So you get them in the project if you use add_subdirectory (or generate and open Boost.sln), but you don't when Boost is installed. Supposing one would like to make the headers appear in the installed project as well, one would add them as PUBLIC sources too. But then, their paths will not be adjusted because we only do this for things in extra/ (and only when their extension is .natvis, which seems a bit unprincipled.)

Maybe we should also apply the same treatment to (a) sources in include/ and (b) sources in extra/ that aren't .natvis.

(Keeping the scope of this patch limited to extra/**/*.natvis was correct; I'm saying that we should consider expanding it, maybe for the release after the next.)

@pdimov
Copy link
Member

pdimov commented Oct 18, 2025

I also wonder why I'm checking for CMake 3.19 here

https://github.com/boostorg/assert/blob/5dcb2af5213ae132b7531e45e7f93258cc33ffd8/CMakeLists.txt#L20

Perhaps the CMake documentation was wrong when I looked at it, because now neither target_sources on an imported target, nor source_group(TREE seem to need 3.19.

Also, given that the original issue that prompted all this wants vcpkg to install the .natvis files, I wonder whether the check for the Visual Studio generator isn't too conservative, because vcpkg uses Ninja.

@k3DW k3DW deleted the natvis branch October 18, 2025 22:50
@k3DW
Copy link
Contributor Author

k3DW commented Oct 18, 2025

Maybe we should also apply the same treatment to (a) sources in include/ and (b) sources in extra/ that aren't .natvis.

I think I agree with that, depending on whether the installed headers would be transitively added to any VS projects that link to the installed Boost library. Definitely worth a discussion, and looking into further.

I also wonder why I'm checking for CMake 3.19 here

I was wondering that a while back too, and I assumed that it was for source_group. From the docs:

Added in version 3.18: Allow using forward slashes (/) to specify subgroups.

Also, given that the original issue that prompted all this wants vcpkg to install the .natvis files, I wonder whether the check for the Visual Studio generator isn't too conservative, because vcpkg uses Ninja.

I think it will work as-is. I don't see the check for the VS generator, I see a check for MSVC here. That should work regardless of generator.

@pdimov
Copy link
Member

pdimov commented Oct 18, 2025

https://github.com/boostorg/unordered/blob/e01b7b52a6ac1ea462aa470a52cb08a879903ff7/CMakeLists.txt#L26

@k3DW
Copy link
Contributor Author

k3DW commented Oct 19, 2025

Got it. Then yes, I agree that this check is too conservative, and it should probably check for MSVC instead. As far as I know, the Visual Studio debugger can be used on all MSVC binaries, not just ones compiled with MSBuild. So then the Nativs files should included in all cases of MSVC. I'll make a suggested edit on my related Unordered PR.

@pdimov
Copy link
Member

pdimov commented Oct 19, 2025

I am experimenting with adding the sources unconditionally for Assert, to see what would happen.

I had to change the boost_install logic to not be limited to if(MSVC), because if INTERFACE_SOURCES contains a source file rooted in extra/, this source file needs the adjustment regardless of whether MSVC is defined, otherwise there's an error that it's being installed while rooted in the source directory:

https://github.com/boostorg/assert/actions/runs/18620181915/job/53090281688

CMake Error in libs/assert/CMakeLists.txt:
  Target "boost_assert" INTERFACE_SOURCES property contains path:

    "/home/runner/work/assert/boost-root/libs/assert/extra/boost_assert.natvis"

  which is prefixed in the source directory.

So I did:

466ba5e

Incidentally, this is still not wide enough, as it only works for sources immediately in extra/ but not a subdirectory of extra/. So far we don't have any, but we might, in the future. If you're willing to work on that, we still have time before the feature freeze.

And now (thanks to Alexander Grund's CI improvements) I have the answer to why I wanted CMake 3.19 in the check:

https://github.com/boostorg/cmake/actions/runs/18626891304/job/53106142673

CMake Error at libs/assert/CMakeLists.txt:24 (target_sources):
  target_sources may only set INTERFACE properties on INTERFACE targets


CMake Error at libs/assert/CMakeLists.txt:27 (target_sources):
  target_sources may only set INTERFACE properties on INTERFACE targets

I wonder what's the right thing here, switch to INTERFACE sources, or add back the version check (with a comment that explains why it's there :-)).

@pdimov
Copy link
Member

pdimov commented Oct 19, 2025

Oh, but using INTERFACE sources for the header files will require that we fixup them in boost_install as well. So I'll add back the version check, for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants